Fix bug where ASan shadow region was overlapping static data - #15806
Conversation
37a0c08 to
ea1d60e
Compare
This bug was caused by miscalculating the adjustments make to TOTAL_MEMORY and GLOBAL_BASE and using ASan. The bug was introduced in #14631 and was causing ASan to write poison values over static data in some cases. This bug was really only effecting builds without ALLOW_MEMORY_GROWTH since when ALLOW_MEMORY_GROWTH is enabled that shadow region is very large and unless users were use a lot of memory it was unlikely ASan would write to the end of it (and clobber static data). As well as fixing the calculations, this change also adds a runtime assert to ensure these regions never overlap (at least not at startup). This should catch regressions in these calculations. In order to test this case I removed ALLOW_MEMORY_GROWTH form several asan and lsan tests. It should not longer be necessary to set `ALLOW_MEMORY_GROWTH` or adjust `TOTAL_MEMORY` when using sanitizers these days. The `asan` suite in the core tests still uses `ALLOW_MEMORY_GROWTH`. As a followup we might want to look into removing that too. Fixes: #15762
ea1d60e to
b74634b
Compare
ffb13fe to
0296f5b
Compare
| # to find the total memory and the shadow size based on the fact | ||
| # that the user memory is 7/8ths of the total memory. | ||
| # (i.e. user_mem == total_mem * 7 / 8 | ||
| total_mem = user_mem * 8 / 7 |
There was a problem hiding this comment.
I am slightly worried about rounding errors here, since I assume ASan rounds up, that is, isn't the size of shadow memory potentially slightly more than user memory / 7?
I would compute this is
shadow_mem = ..however asan rounds..
total_mem = user_mem + shadow_memOr does ASan actually compute it as a fraction of total memory, not user memory? In that case your code lgtm
There was a problem hiding this comment.
ASan literally does a shift to from a pointer to its shadow byte:
The problem with trying to start by calculating the size of the shadow region is that you can't do that without also knowing the size of the shadow region.. since the bigger the shadow region the more shadow region you need because it shift the reset of the memory upwards as it grows.
There was a problem hiding this comment.
The assertion that I've added to the code I think means that if these calculation are wrong we would know right away.. at least if we ever under-estimate the size of the shadow region which is the dangerous direction.
There was a problem hiding this comment.
Wait if SHADOW_SCALE=3 then total memory is 9/8 times user memory, isn't it, and not 8/7?
There was a problem hiding this comment.
Because SHADOW_SCALE=3 the size of the shadow memory is 1/8th size of the total memory. But shadow memory is also part of the total memory and because shadow memory leaves at address zero that leave 7/8ths of the total memory left, for what we are calling user memory.
There was a problem hiding this comment.
For what its worth the calculation of __global_base in emcc.py is puts its exactly at the address calculated for max_shadow_address in the native code.. which seems like fairly good evidence that the calculation here are correct.
0296f5b to
1b4df02
Compare
1b4df02 to
69477c0
Compare
|
Landing despite unrelated browser.test_emscripten_async_wget2_data failure. That looks like flakyness on the part of the remote host used by that test. |
…ten-core#15806) This bug was caused by miscalculating the adjustments made to TOTAL_MEMORY and GLOBAL_BASE and using ASan. The bug was introduced in emscripten-core#14631 and causes ASan to write poison values over static data in some cases. This bug was really only effecting builds without ALLOW_MEMORY_GROWTH since when ALLOW_MEMORY_GROWTH is enabled that shadow region is very large and unless users were use a lot of memory it was unlikely ASan would write to the end of it (and clobber static data). As well as fixing the calculations, this change also adds a runtime assert to ensure these regions never overlap (at least not at startup). This should catch regressions in these calculations. In order to test this case I removed ALLOW_MEMORY_GROWTH form several asan and lsan tests. It should not longer be necessary to set `ALLOW_MEMORY_GROWTH` or adjust `TOTAL_MEMORY` when using sanitizers these days. The `asan` suite in the core tests still uses `ALLOW_MEMORY_GROWTH`. As a followup we might want to look into removing that too. Fixes: emscripten-core#15762
3 participants
This bug was caused by miscalculating the adjustments made to TOTAL_MEMORY
and GLOBAL_BASE when using ASan. The bug was introduced in #14631 and
was causing ASan to write poison values over static data in some cases.
This bug was really only effecting builds without ALLOW_MEMORY_GROWTH
since when ALLOW_MEMORY_GROWTH is enabled that shadow region is very
large and unless users were use a lot of memory it was unlikely ASan
would write to the end of it (and clobber static data).
As well as fixing the calculations, this change also adds a runtime
assert to ensure these regions never overlap (at least not at startup).
This should catch regressions in these calculations.
In order to test this case I removed ALLOW_MEMORY_GROWTH form several
asan and lsan tests. It should not longer be necessary to set
ALLOW_MEMORY_GROWTHor adjustTOTAL_MEMORYwhen using sanitizersthese days. The
asansuite in the core tests still usesALLOW_MEMORY_GROWTH. As a followup we might want to look intoremoving that too.
Fixes: #15762